Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 filter global namespace while looking for cluster scoped resources #1520

Merged
merged 2 commits into from
May 26, 2021
Merged

🐛 filter global namespace while looking for cluster scoped resources #1520

merged 2 commits into from
May 26, 2021

Conversation

varshaprasad96
Copy link
Member

In the current implementation of multinamespaced cache, we look at
global namespace while listing objects from all namespaced. Global namespace
should not be looked at while fetching namespaced resources.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2021
@k8s-ci-robot k8s-ci-robot requested review from droot and joelanford May 8, 2021 18:08
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 8, 2021
@varshaprasad96
Copy link
Member Author

In the GET calls, the namespace of the object should be populated right (here) since its taken from the ObjectKey, so I believe calling of global cache wouldn't occur.
I think this should solve the bug caused from PR #1418

@varshaprasad96 varshaprasad96 mentioned this pull request May 8, 2021
Merged
@alvaroaleman alvaroaleman added this to the v0.9.x milestone May 10, 2021
@estroz
Copy link
Contributor

estroz commented May 10, 2021

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 10, 2021
@estroz
Copy link
Contributor

estroz commented May 10, 2021 •
edited
Loading

Also a potential source of future bugs is this line, where the global cache non-namespace _global_cache gets passed to the informer constructor. Within informer code, namespacing is enforced by both != "" and scope checks, so the non-namespace does not actually cause issues now. However if internal code uses namespace directly in the future without a scope check, a bug could be introduced. I recommend setting opts.Namespace = "" if the global cache namespace is the iterate, or better yet, construct it separately above the loop.

@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from acb67ae to 49b9d0a Compare May 13, 2021 18:39
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 13, 2021
@varshaprasad96
Copy link
Member Author

Have moved the global cache creation above the loop, so that the _cluster-scope namespace is not passed to opts.Namespace. In a follow up PR, will refactor this to not add global cache to the map itself, since as Eric mentioned, the namespace to cache map assumes that the namespaces are actually valid.

@sbueringer sbueringer mentioned this pull request May 19, 2021
Merged
@varshaprasad96
🐛 filter global namespace while looking for cluster scoped resources
50bfd90 
In the current implementation of multinamespaced cache, we look at
global namespace while listing objects from all namespaced. Global namespace
should not be looked at while fetching namespaced resources.
@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from 49b9d0a to 56f0e9b Compare May 25, 2021 02:12
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2021
@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from 56f0e9b to b865173 Compare May 25, 2021 02:18
varshaprasad96
varshaprasad96 commented May 25, 2021
View reviewed changes
pkg/cache/multi_namespace_cache.go Outdated
if err != nil {
return nil, err
}
informers[globalCache] = clusterCacheInf
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could ideally create a separate informer in multiNamespaceInformer for cluster scoped cache. But that's not needed for now, since the namespace is not specifically used in any of the methods.

@varshaprasad96
Copy link
Member Author

/retest

1 similar comment
@estroz
Copy link
Contributor

estroz commented May 25, 2021

/retest

alvaroaleman
alvaroaleman reviewed May 25, 2021
View reviewed changes
pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved
pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved
@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from b865173 to 412bf7a Compare May 25, 2021 18:53
@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from 412bf7a to 18e3e67 Compare May 25, 2021 19:36
@varshaprasad96 varshaprasad96 force-pushed the fix/multinamespaced_cache branch from 18e3e67 to 201ab80 Compare May 25, 2021 23:21
estroz
estroz approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021
@alvaroaleman alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 26, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, estroz, varshaprasad96

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 12f94f1 into kubernetes-sigs:master May 26, 2021
@varshaprasad96 varshaprasad96 mentioned this pull request Mar 27, 2023
Merged
varshaprasad96 added a commit to varshaprasad96/controller-runtime that referenced this pull request Mar 27, 2023
@varshaprasad96
🐛 fix a bug in multinamespaced cache implementation
83cc439 
This commit fixes a bug that was brought in long ago in kubernetes-sigs#1520.
When the object's scope is not deterministic from the RESTMapper
it should return an error instead of ignoring it.

Signed-off-by: Varsha Prasad Narsing <[email protected]>
k8s-infra-cherrypick-robot pushed a commit to k8s-infra-cherrypick-robot/controller-runtime that referenced this pull request Apr 24, 2023
@varshaprasad96
bug: fix a bug in multinamespaced cache implementation
57b0968 
This commit fixes a bug that was brought in long ago in kubernetes-sigs#1520.
When the object's scope is not deterministic from the RESTMapper
it should return an error instead of ignoring it.

Signed-off-by: Varsha Prasad Narsing <[email protected]>
alebedev87 pushed a commit to alebedev87/controller-runtime that referenced this pull request Apr 24, 2023
@varshaprasad96 @alebedev87
🐛 fix a bug in multinamespaced cache implementation
03d09f6 
This commit fixes a bug that was brought in long ago in kubernetes-sigs#1520.
When the object's scope is not deterministic from the RESTMapper
it should return an error instead of ignoring it.

Signed-off-by: Varsha Prasad Narsing <[email protected]>
@alebedev87 alebedev87 mentioned this pull request Apr 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@droot droot Awaiting requested review from droot

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@alvaroaleman alvaroaleman

@estroz estroz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v0.9.x
Development

Successfully merging this pull request may close these issues.
4 participants
@varshaprasad96 @estroz @k8s-ci-robot @alvaroaleman